-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
API and implementation of codefixes and code actions for Rascal itself #478
base: main
Are you sure you want to change the base?
Conversation
@DavyLandman @toinehartman this is still a draft but if you are interested and perhaps have feedback... that would be nice. I'm trying to prepare us for the inevitable removed of |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice improvement! I like how this allows to write transformations and other code actions relatively easily. I am curious how you'll tackle contribution code actions from the type checker!
I was planning on learning that from you @toinehartman But for the actions that can be attached to an error or warning message, it will be simply a The only small problem is that the type definitions for
|
…Becomes functionn from dsl tests
…hird option in the menu
… that adds a license
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like how most of it looks 👍🏼 great reuse of the existing code-actions added the previous PR. My only concern is with the extra evaluator.
rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/rascal/RascalLanguageServices.java
Outdated
Show resolved
Hide resolved
rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/rascal/RascalLanguageServices.java
Show resolved
Hide resolved
rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/rascal/RascalLanguageServices.java
Outdated
Show resolved
Hide resolved
rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/rascal/RascalLanguageServices.java
Outdated
Show resolved
Hide resolved
…with respect to code actions
I think this is ready if you do too @toinehartman @DavyLandman. Note that this does not test any quickfix CodeActions attached to error or warning messages from the checker, yet. That's because the checker does not produce them yet. I plan to add a test here anway, as soon as one of those quickfixes starts popping up by the checker. But that's after merging IMHO. |
Quality Gate passedIssues Measures |
I fixed the evaluator but the PR still thinks this is open? Help, where do I click? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small remarks after a second review. I've changed my vote to approve, such that if you've went through them, and dealt with them to your discretion, you can merge.
@@ -108,11 +110,12 @@ public void didChangeWorkspaceFolders(DidChangeWorkspaceFoldersParams params) { | |||
|
|||
@Override | |||
public CompletableFuture<Object> executeCommand(ExecuteCommandParams params) { | |||
if (params.getCommand().startsWith(RASCAL_META_COMMAND)) { | |||
if (params.getCommand().startsWith(RASCAL_META_COMMAND) || params.getCommand().startsWith(RASCAL_COMMAND)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if for RASCAL_COMMAND
we also want to do this? as this is about delegating to the sub-language, but that doesn't apply for the rascal commands.
Shouldn't that be a separate branch? that only runs documentService.execute
command directly? or are we always passing in the rascal-langauge name everytime?
.map(cmd -> Either.<Command,CodeAction>forRight(cmd)) | ||
.collect(Collectors.toList()) | ||
); | ||
return CodeActions.mergeAndConvertCodeActions(this, dedicatedLanguageName, contribs.getName(), quickfixes, codeActions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good that these got extracted out into a helper class.
// convert to Rascal 1-based line | ||
final var startLine = start.getLine() + 1; | ||
// convert to Rascal UTF-32 column width | ||
final var startColumn = columns.get(loc).translateInverseColumn(start.getLine(), start.getCharacter(), false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm positive I've seen this code before, can we reduce the duplication? especially as it's easy to mess this up.
} | ||
|
||
private CompletableFuture<IList> computeCodeActions(final int startLine, final int startColumn, ITree tree, PathConfig pcfg) { | ||
IList focus = TreeSearch.computeFocusList(tree, startLine, startColumn); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please move this calculation of the focus list into the completeable future, to move as much of the calculation to async task.
|
||
// finds an open menu with the right item in it (Change to a) and then select | ||
// the parent that handles UI events like click() and sendKeys() | ||
const menuContainer = await ide.hasElement(editor, By.xpath("//div[contains(@class, 'focused') and contains(@class, 'action')]/span[contains(text(), 'Add missing license header')]//ancestor::*[contains(@class, 'monaco-list')]"), Delays.normal, "Add-license action should be available and focused"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should refactor this into a function into the IDE class, with the text as an argument.
Command
andCodeAction
definitions to rascal-core, such that the checker can annotate messages with quickfixesRascalActions.mov